use rtos semaphore as mutex in esp-radio refcount - #6329
Open
tommasoclini wants to merge 8 commits into
Open
tommasoclini wants to merge 8 commits into
tommasoclini wants to merge 8 commits into
Conversation
tommasoclini
changed the title
tommasoclini
marked this pull request as ready for review
September 17, 2026 16:41
tommasoclini
requested review from
JurajSadel,
MabezDev and
bjoernQ
as code owners
September 17, 2026 16:41
bugadani
reviewed
Sep 21, 2026
Comment on lines
+32
to
+67
| fn use_sem_or_init<T>(&self, f: impl FnOnce(&SemaphoreHandle) -> T) -> T { | ||
| if self.sem.load(Ordering::Relaxed).is_null() { | ||
| core::hint::cold_path(); | ||
|
|
||
| let sem = SemaphoreHandle::new(SemaphoreKind::Mutex).leak(); | ||
|
|
||
| if self | ||
| .sem | ||
| .compare_exchange( | ||
| null_mut(), | ||
| sem.as_ptr(), | ||
| Ordering::Release, | ||
| Ordering::Relaxed, | ||
| ) | ||
| .is_err() | ||
| { | ||
| core::hint::cold_path(); | ||
|
|
||
| drop(unsafe { SemaphoreHandle::from_ptr(sem) }); | ||
| } | ||
| } | ||
|
|
||
| let sem = unsafe { SemaphorePtr::new_unchecked(self.sem.load(Ordering::Acquire)) }; | ||
| f(unsafe { SemaphoreHandle::ref_from_ptr(&sem) }) | ||
| } | ||
|
|
||
| fn try_use_sem<T>(&self, f: impl FnOnce(&SemaphoreHandle) -> T) -> Option<T> { | ||
| if self.sem.load(Ordering::Relaxed).is_null() { | ||
| core::hint::cold_path(); | ||
|
|
||
| None | ||
| } else { | ||
| let sem = unsafe { SemaphorePtr::new_unchecked(self.sem.load(Ordering::Acquire)) }; | ||
| Some(f(unsafe { SemaphoreHandle::ref_from_ptr(&sem) })) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
I don't see much point in splitting these apart. Both are single-use wrappers, and they do roughly the same thing. If you merge them with the call sites, at least one layer of the callback lasagna would go away.
bugadani
reviewed
Sep 21, 2026
| }) | ||
| } | ||
|
|
||
| fn try_lock<T>(&self, f: impl FnOnce(&mut u32) -> T) -> Option<T> { |
Contributor
There was a problem hiding this comment.
I think I'd just get rid of try_lock entirely, I don't see the value in it. Yes, it's wasteful to initialize a mutex if we are going to panic anyway, but now I have to figure out why this even exists and what the intended use case is. From what I can see, lock can be used in all the 1 callsites.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Submission Checklist 📝
cargo xtask fmtcommand to ensure that all changed code is formatted correctly.skip-changelogormanual-changeloglabel as appropriate.Extra:
Pull Request Details 📖
Description
Use an
esp-radio-rtos-drivermutex inesp_radio::refcount::Refcountto avoid spinning when synchronizing deinitialization and initialization.Testing
todo
Changelog
esp-radio
refcount::Refcountnow usesesp_radio_rtos_driver's api to avoid spinning, now on wrong decrement calls or counter overflow a panic takes place.